Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New C++ custom ODEs example #922

Merged
merged 3 commits into from
Feb 23, 2021
Merged

Conversation

paulblum
Copy link
Contributor

Proposing the addition of a new example for Cantera's C++ sample code, custom.cpp. This example solves a closed-system constant-pressure ignition problem using custom-implemented ODE governing equations, which are solved by CVODES. The actual problem is equivalent to the one solved in the existing custom.py example, and produces the same results.

This example was created as part of my GSoC project, Developing a 0-D Steady-State Combustion Solver for Cantera. The project is ongoing, and progress updates can be found in the enhancements repository.

@ischoegl
Copy link
Member

ischoegl commented Sep 2, 2020

@paulblum ... great to see a new example! The current CI failures should be relatively easy to fix by specifying thresholds (just had to fix a similar thing for #927 - I admittedly didn't investigate and just copied values).

Beyond, I think it may make sense to switch the mechanism from gri30.yaml to h2o2.yaml here and for custom.py (other than not having N2 - which is replaceable by AR for the purpose of an example - this shouldn't change much).

@bryanwweber
Copy link
Member

@ischoegl Any reason to prefer h2o2.yaml over gri30.yaml? I doubt there's any difference significant difference in how long they take to run on modern processors.

@ischoegl
Copy link
Member

ischoegl commented Sep 2, 2020

@bryanwweber ... it's admittedly a minor (philosophical?) point, but things do add up. I would generally advocate for using the smallest possible mechanism that will do the job especially when there is absolutely no benefit of using a larger mechanism: as h2o2.yaml is available (as GRI minus carbon/nitrogen), it's the logical candidate for probing a H2-O2 system for the sake of an example.

PS: As another benefit, end-users (who are probably interested in some C species) will have to actually think about what mechanism to use instead of always defaulting to gri30.yaml - I believe we all agree that there are better options out there.

@paulblum
Copy link
Contributor Author

paulblum commented Sep 2, 2020

Just updated the regression test tolerances from the defaults to threshold=1e-10 and tolerance=2e-4, which match the ones specified for cxx-combustor.

@paulblum
Copy link
Contributor Author

paulblum commented Sep 2, 2020

@ischoegl thanks for the feedback! I'll look into switching this mechanism next.

@bryanwweber
Copy link
Member

@paulblum @ischoegl I'd be in favor of deferring the switch to h2o2.yaml to a separate pull request, since it could be done for several examples, and leaving this with gri30.yaml means it'll match the Python example until such time as we switch any other examples. Doing a PR to change over the example input files would be a good first issue for any new contributors as well.

@ischoegl
Copy link
Member

ischoegl commented Sep 4, 2020

🤷‍♂️ ... makes no difference as long as it’ll get done. I agree that this is an easy fix ... 5ish lines spread over a couple of files? Not sure it’s worth its own PR ...

PS: #932

Copy link
Member

@speth speth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for providing this example, @paulblum! I think it looks very good, and provides a C++ example that's non-trivial but still fairly clean.

I think there are just a handful of issues to tweak, along with rebasing this and fixing the conflicts.

samples/cxx/custom/custom.cpp Outdated Show resolved Hide resolved
samples/cxx/custom/custom.cpp Show resolved Hide resolved
samples/cxx/custom/custom.cpp Outdated Show resolved Hide resolved
samples/cxx/custom/custom.cpp Show resolved Hide resolved
samples/cxx/custom/custom.cpp Outdated Show resolved Hide resolved
samples/cxx/custom/custom.cpp Outdated Show resolved Hide resolved
samples/cxx/custom/custom.cpp Outdated Show resolved Hide resolved
samples/cxx/custom/custom.cpp Outdated Show resolved Hide resolved
samples/cxx/custom/custom.cpp Outdated Show resolved Hide resolved
@paulblum
Copy link
Contributor Author

Thanks for the review @speth, these were all really good suggestions! Just pushed a commit with the updates, please let me know if there's anything else you think can be improved 😄

@paulblum paulblum requested a review from speth February 23, 2021 00:34
@paulblum
Copy link
Contributor Author

Failed CI test due to regression test tolerances, I had fixed that in an earlier version but looks like it got removed by a conflict... latest commit should be all set!

@speth
Copy link
Member

speth commented Feb 23, 2021

Thanks, @paulblum, this looks great. I think the only thing I'd like to do before merging is to squash the last couple of commits to avoid the large diff associated with the changes to the custom_cxx_blessed.csv file. If you're comfortable doing that, please go ahead, otherwise I can do it.

@paulblum
Copy link
Contributor Author

@speth that makes sense to me, although I'm not familiar with how to squash commits... probably better for you to do it if it's not too much trouble. Thanks again for the help!

Switch from arrays to vectors, and incorporate review suggestions
@speth speth merged commit e70371a into Cantera:main Feb 23, 2021
@paulblum paulblum deleted the C++Example-CustomODEs branch February 26, 2021 15:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants